-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(kms) default key policy: give all permissions to root account #2329
Conversation
It might be that you have a problem, but this is not the way to fix it. This gives all IAM users/roles in your account permissions to do ANYTHING to the key. You are free to use this in your own application (though I would advise against it), but we need more fine-grained permissions than that in the library, both in terms of API calls allowed as well as principals they are allowed to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Permissions are unacceptably widened this way.
In fact, just seeing this PR open makes me eerily uncomfortable, for fear of someone merging it accidentally. It's like standing at the edge of a cliff face :). I'm sorry, but I'm going to close this. Feel free to reopen once the code gets changed. |
Hey @rix0rrr , thanks for having a look at my PR. I want to clear out some points:
This is false. The KMS key policy generated by this pull request gives full access only to the root user.
In fact, using such a default key policy is a practice recommended by the KMS documentation: https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam . Furthermore, if you create a KMS key manually from the console, and leave everything to default values, the exact same policy will be generated. To verify my claim that such a key policy does not give access to every user/role in the current AWS account, you can deploy the following stack: ---
AWSTemplateFormatVersion: '2010-09-09'
Resources:
MyKey:
Type: AWS::KMS::Key
Properties:
KeyPolicy:
Version: '2012-10-17'
Statement:
- Sid: Allow root user to manage key
Effect: Allow
Principal:
AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
Action: kms:*
Resource: "*"
UserWithoutAccess:
Type: AWS::IAM::User
UserWithAccess:
Type: AWS::IAM::User
Properties:
Policies:
- PolicyName: KMSaccess
PolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Action:
- kms:Encrypt
Resource:
- !GetAtt MyKey.Arn Once this stack is deployed, create access keys for both users. Using credentials for
However, using credentials for
|
Hi SP, Thanks for getting back to me. Apparently I misunderstood what the root account means in this context, but to be fair, I think its meaning deviates from other IAM policies? I will pass this on to KMS team and security to see what they have to say. In any case, doesn't it make more sense for the |
By the way, I'm still confused; if your example is correct and UserWithoutAccess does NOT have permissions to the key given this policy... then how could it have made any difference to the user/role pulling from your encrypted queue? |
Hey Rico, I made a few experiments. Here are my conclusions. In all cases, for the Lambda Function to be able to receive messages from the encrypted queue, const myFunction = new lambda.Function(this, 'MyFunction', {
...
initialPolicy: [
new iam.PolicyStatement()
.addAction('kms:Decrypt')
.addResource(queueKey.keyArn)
],
}); Assuming the function above, I have found only 2 solutions that allow my Lambda function to receive messages from the encrypted queue: solution 1: Give all permissions to the root account on the key's policy (exactly what this PR does) KeyPolicy:
Version: '2012-10-17'
Statement:
- Sid: Allow root user to manage key
Effect: Allow
Principal:
AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
Action: kms:*
Resource: "*" solution 2: Add the const queueKey = new kms.EncryptionKey(this, 'QueueKey', {
enableKeyRotation: true,
});
queueKey.addToResourcePolicy(
new iam.PolicyStatement()
.addAccountRootPrincipal()
.addActions('kms:GenerateDataKey*', 'kms:Decrypt')
.addAllResources()
); Here are other failed attempts that did not allow my Lambda function to receive messages from the encrypted queue: failed 1: Add the const queueKey = new kms.EncryptionKey(this, 'QueueKey', {
enableKeyRotation: true,
});
queueKey.addToResourcePolicy(
new iam.PolicyStatement()
.addPrincipal(new iam.ServicePrincipal(`lambda.${cdk.Aws.urlSuffix}`))
.addActions('kms:GenerateDataKey*', 'kms:Decrypt')
.addAllResources()
); failed 2: Giving all const myFunction = new lambda.Function(this, 'MyFunction', {
...
initialPolicy: [
new iam.PolicyStatement()
.addAction('kms:*')
.addResource(queueKey.keyArn)
],
}); I did not find any documentation on setting up a Lambda function to receive messages from an encrypted SQS queue. If you have ideas for other solutions please let me know. |
I think it's going to boil down to Can you try making it so that the |
Has your issue potentially been solved by #2659 ? |
Yes, #2659 fixed it! Thanks a lot :) |
Enables all
kms:*
actions on the KMS key's default policy.Context:
I was trying to trigger a Lambda function to receive messages from an encrypted (customer-managed KMS key) SQS queue.
When using the default policy for the KMS key:
the Lambda function was not able to receive messages from the encrypted SQS Queue. To fix this problem, I had to override the KMS key's default policy:
Using the above fix, the Lambda function was able to receive messages from the encrypted SQS Queue.
Source: https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.